Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Implement IntentFactory (v1.0.0) #705

Draft
wants to merge 34 commits into
base: main
Choose a base branch
from
Draft

Implement IntentFactory (v1.0.0) #705

wants to merge 34 commits into from

Conversation

ezynda3
Copy link
Contributor

@ezynda3 ezynda3 commented Jul 4, 2024 •
edited
Loading

Which Jira task belongs to this PR?

LF-6656

Why did I implement it this way?

Checklist before requesting a review

  • I have performed a self-review of my code
  • This pull request is as small as possible and only tackles one problem
  • I have added tests that cover the functionality / test the bug
  • I have updated any required documentation
  • If this requires a contract version change, I have updated the version number in the source file

Checklist for reviewer (DO NOT DEPLOY any contracts BEFORE CHECKING THIS!!!)

  • I have checked that any arbitrary calls to external contracts are validated and or restricted
  • I have checked that any privileged calls (i.e. storage modifications) are validated and or restricted
  • I have ensured that any new contracts have had AT A MINIMUM 1 preliminary audit conducted on by <company/auditor>

@ezynda3 ezynda3 added the WIP Work in progress label Jul 4, 2024
@ezynda3 ezynda3 marked this pull request as ready for review July 10, 2024 12:18
@ezynda3 ezynda3 added In Review and removed WIP Work in progress labels Jul 10, 2024
0xDEnYO
0xDEnYO requested changes Jul 16, 2024
View reviewed changes
docs/IntentFactory.md Show resolved Hide resolved
src/Helpers/Intent.sol Outdated
/// @author LI.FI (https://li.fi)
/// @notice Intent contract that can execute arbitrary calls.
/// @custom:version 1.0.0
contract Intent {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not a big fan of the naming.
Would rather go for something like "IntentHandler" or anything that describes what this contract does with an intent. I find it misleading to call it an intent since it also does the handling of execution and withdrawal and not just represent an intent

script/deploy/facets/DeployIntentFactory.s.sol Outdated Show resolved Hide resolved
script/demoScripts/demoIntentFactory.ts Outdated Show resolved Hide resolved
script/demoScripts/demoIntentFactory.ts Outdated Show resolved Hide resolved
test/solidity/Periphery/IntentFactory.t.sol Outdated Show resolved Hide resolved
test/solidity/Periphery/IntentFactory.t.sol
import { TestToken } from "../utils/TestToken.sol";
import { TestAMM } from "../utils/TestAMM.sol";

contract IntentFactoryTest is Test {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am missing negative test cases in the file.
Stuff like "not anyone can execute / withdraw".
Or "cannot execute any other calldata".
Or "will fail if minAmount not reached"

If/once an event has been added to the Intentcontract this should be correctly caught here, too.

src/Helpers/Intent.sol Outdated Show resolved Hide resolved
src/Helpers/Intent.sol Outdated Show resolved Hide resolved
src/Helpers/Intent.sol Outdated Show resolved Hide resolved
@0xDEnYO 0xDEnYO marked this pull request as draft August 1, 2024 01:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0xDEnYO 0xDEnYO 0xDEnYO requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
In Review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ezynda3 @0xDEnYO